feat: add blast radius methodlogy (CM-1328)#4377
Conversation
PR SummaryHigh Risk Overview Akrites-external gains
Reviewed by Cursor Bugbot for commit 00de0ed. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
Implements the npm blast-radius pipeline with database-backed state, Temporal stages, agent analysis, and public result polling.
Changes:
- Adds schema and DAL operations for analyses, dependents, verdicts, and stage runs.
- Implements intelligence, dependent scanning, reachability, and reporting stages.
- Adds supporting npm/OSV clients, agent prompts, semver utilities, and API responses.
Metadata: Rename the title to feat: add blast radius methodology (CM-1328). The PR also exceeds the recommended 1,000-line target.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
services/libs/data-access-layer/src/packages/blastRadius.ts |
Adds pipeline persistence operations. |
services/apps/packages_worker/src/blast-radius/stages/report.ts |
Finalizes analyses and costs. |
services/apps/packages_worker/src/blast-radius/stages/reachability.ts |
Analyzes dependent reachability. |
services/apps/packages_worker/src/blast-radius/stages/intel.ts |
Produces vulnerability symbol intelligence. |
services/apps/packages_worker/src/blast-radius/stages/dependents.ts |
Persists dependent scan results. |
services/apps/packages_worker/src/blast-radius/semverRange.ts |
Adds semver matching helpers. |
services/apps/packages_worker/src/blast-radius/semverRange.test.ts |
Tests semver helpers. |
services/apps/packages_worker/src/blast-radius/npmManifest.ts |
Extends npm manifest typing locally. |
services/apps/packages_worker/src/blast-radius/dependentsScan.ts |
Discovers and ranks npm dependents. |
services/apps/packages_worker/src/blast-radius/clients/osvClient.ts |
Fetches and interprets OSV records. |
services/apps/packages_worker/src/blast-radius/clients/npmTarball.ts |
Downloads and extracts npm sources. |
services/apps/packages_worker/src/blast-radius/clients/npmAbbreviated.ts |
Fetches abbreviated npm metadata. |
services/apps/packages_worker/src/blast-radius/clients/githubPatch.ts |
Downloads GitHub patches. |
services/apps/packages_worker/src/blast-radius/agent/runner.ts |
Configures Agent SDK execution. |
services/apps/packages_worker/src/blast-radius/agent/prompts.ts |
Defines agent prompts and schemas. |
services/apps/packages_worker/src/blast-radius/activities.ts |
Exposes Temporal activities. |
backend/src/osspckgs/migrations/V1784700000__blast_radius_analyses.sql |
Creates blast-radius tables and indexes. |
backend/src/api/public/v1/packages/getBlastRadiusJob.ts |
Adds analysis polling endpoint. |
backend/src/api/public/v1/packages/getBlastRadiusJob.test.ts |
Tests polling behavior. |
backend/src/api/public/v1/packages/blastRadiusAnalysis.ts |
Maps stored results to API responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
99915ef to
fbb843b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
backend/src/api/public/v1/packages/blastRadiusAnalysis.ts:95
- This overstates
dependentsExcludedUpfront.candidatesConsideredis every phase-1 hit, while phase 2 stops aftertopNin-range packages and also skips packument failures; therefore the difference includes candidates that were never range-checked. For example, 100 hits with the first 25 in range reports 75 exclusions even though none were excluded. Persist/use the actual excluded count (and distinguish skipped/unprocessed candidates) when building the summary.
totalDependentsInRange,
dependentsExcludedUpfront: Math.max(totalDependentsInRange - dependentsAnalyzed, 0),
services/apps/packages_worker/src/blast-radius/clients/npmTarball.ts:34
strict/preservePathsprotects archive entry paths, but it does not make symlink targets safe. A malicious tarball can extract a symlink to a file outsidetempDir;copyTreeGuardedlater usesstatSyncandcopyFileSync, both of which follow that link, copying host files into the agent workspace. Reject symbolic/hard-link entries during extraction, or uselstatplus realpath containment before copying.
const extractStream = tar.extract({
cwd: tempDir,
strict: true,
}) as unknown as NodeJS.WritableStream
services/apps/packages_worker/src/blast-radius/stages/reachability.ts:138
- The stage cost omits billed failed attempts.
runAnalysisAgentreturnstotal_cost_usdeven for non-success result messages, but this adds cost only after a successful attempt. If an attempt consumes tokens and a retry succeeds, the finalized analysis under-reports actual spend; accumulate each attempt's cost exactly once before deciding whether to retry.
results.cost += agentResult.costUsd || 0
fbb843b to
fb94246
Compare
45f337d to
b664863
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 11 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (4)
services/apps/packages_worker/src/blast-radius/stages/intel.ts:67
- When a package was explicitly requested but is not present in the advisory, this silently falls back to the first npm entry. The completed job then reports results for a different package while echoing the caller's requested package. Only fall back for advisory-wide requests; reject an unmatched explicit package.
const entry =
(requestedPackage && npmEntries.find((e) => e.package.name === requestedPackage)) ||
npmEntries[0]
services/apps/packages_worker/src/blast-radius/agent/runner.ts:49
- The analyzed npm source is untrusted and can contain prompt-injection instructions. Allowing
Readwhile bypassing all permission prompts lets the agent read paths outsidecwd; the subprocess also inherits the worker's full environment, so injected source can retrieve credentials and return them in persisted/public verdict fields. Run the agent in an OS-level sandbox containing only the extracted tree, restrict file tools to that tree, and pass an allowlisted environment instead of using bypass mode.
tools: ['Read', 'Grep', 'Glob'],
disallowedTools: ['Bash', 'Write', 'Edit', 'NotebookEdit', 'WebFetch', 'WebSearch', 'Task'],
permissionMode: 'bypassPermissions',
allowDangerouslySkipPermissions: true,
services/apps/packages_worker/src/blast-radius/stages/reachability.ts:145
- Only the successful final attempt's cost is persisted here. Any billed failed attempts are discarded, and a persistently failing dependent is stored with cost 0, so
getVerdictsCostand the final analysis total under-report actual spend whenever retries occur. Accumulate and persist cost for every agent result, including failures and activity retries.
costUsd: agentResult.costUsd || 0,
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:166
- All registry errors—including rate limits and transient timeouts—are treated as “not a dependent” here. This scan issues roughly 17k requests at concurrency 32, so temporary failures can silently remove real high-impact dependents and produce a successful but incomplete blast-radius report. Retry transient/rate-limit responses (and fail or report degraded coverage after exhaustion); only
NOT_FOUNDshould be safely skipped.
const packument = await fetchAbbreviatedPackument(name)
if (isFetchError(packument)) return
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 8 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (3)
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:190
- Every registry error is treated as “not a dependent.” With roughly 17k concurrent requests, a 429 or transient outage can therefore remove large portions of the candidate population while the stage still reports success, producing an understated blast radius. Retry/back off
RATE_LIMIT/TRANSIENTfailures or fail the stage; only terminal per-package errors should be skipped.
const packument = await fetchAbbreviatedPackument(name)
if (isFetchError(packument)) return
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:322
- The true
excludedByRangeCountis computed, but only 200 rows are returned and the poll endpoint later derivesdependentsExcludedUpfrontfrom those persisted rows. Analyses with more than 200 exclusions therefore underreport both exclusion and total counts. Persist/use the uncapped count separately while retaining the row cap.
excludedByRange: excludedByRange.slice(0, 200),
excludedByRangeCount: excludedByRange.length,
services/apps/packages_worker/src/blast-radius/stages/reachability.ts:148
- Only the successful attempt's cost is persisted.
runAnalysisAgentalso returnscostUsdfor unsuccessful result messages, so any failed attempts before a successful retry—and all three attempts for an error verdict—are omitted from the final analysis cost. Accumulate cost across attempts and persist that total for both success and terminal failure.
costUsd: agentResult.costUsd || 0,
| ranges?: Array<{ | ||
| type: string | ||
| events?: Array<{ | ||
| introduced?: string | ||
| fixed?: string |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (8)
services/apps/packages_worker/src/blast-radius/stages/intel.ts:70
- When a caller specifies a package that is not present in this advisory, this silently analyzes the first npm entry instead. The poll response still echoes the requested package, so consumers receive results for a different package. Only fall back for advisory-wide requests; reject an unmatched explicit package.
const entry =
(requestedPackage && npmEntries.find((e) => e.package.name === requestedPackage)) ||
npmEntries[0]
services/libs/data-access-layer/src/packages/blastRadius.ts:211
- The
FROM advisoriesinner join prevents this update entirely when the advisory has not yet been synced—the exact nullable-advisory case described by the migration. Consequently, a resolvedpackageIdis also discarded. Resolve the advisory with a scalar subquery sopackage_idis updated independently.
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:296 - A dependency on any related affected package is accepted here, but its declared range is compared against
vulnerableVersionsfor the primary package. Multi-package advisories commonly have unrelated version lines, and Stage 3 also prompts only for the primary package, so these candidates can be incorrectly excluded or analyzed against the wrong symbols. Carry package-specific ranges/specs, or exclude related entries until multi-package handling is implemented.
const depInfo = declaredDependency(manifest, vulnerablePackage, relatedAffectedPackages)
if (!depInfo) continue
const { check, includes } = rangeIncludesAny(depInfo.range, vulnerableVersions)
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:190
- Every registry error—including 429s, timeouts, and 5xx responses—is treated as “not a dependent.” With thousands of concurrent lookups, transient failures therefore produce false negatives while the analysis still completes successfully. Skip only permanent not-found/malformed responses and let transient failures trigger the activity retry.
const packument = await fetchAbbreviatedPackument(name)
if (isFetchError(packument)) return
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:322
- The full exclusion count is computed on the next line but only 200 rows are returned and persisted. The poll endpoint later counts persisted rows, so
dependentsExcludedUpfrontis silently capped at 200 for larger scans. Persist the aggregate count separately (or stop truncating) and use it in the report.
excludedByRange: excludedByRange.slice(0, 200),
excludedByRangeCount: excludedByRange.length,
services/apps/packages_worker/src/blast-radius/clients/npmTarball.ts:49
- Third-party tarballs may contain symbolic or hard links. The later
statSync/copyFileSyncpath follows links, allowing an archive entry to copy host files into the agent workspace despite the destination traversal check. Reject link entries during extraction.
filter: (_entryPath, entry) => {
extractedFiles++
backend/src/api/public/v1/packages/blastRadiusAnalysis.ts:112
dependentsExcludedUpfrontcontains packages whose declared range does not include a vulnerable version, so adding it tototalDependentsInRangemakes that field contradict its name and OpenAPI description. Either report only in-range dependents here or rename/redefine the public field as total candidates evaluated.
return {
totalDependentsInRange: dependentsAnalyzed + dependentsExcludedUpfront,
dependentsExcludedUpfront,
services/libs/data-access-layer/src/packages/osv.ts:139
- This join reconstructs every npm row's full name with a
CASE, so PostgreSQL cannot use the existing unique index on(ecosystem, COALESCE(namespace, ''), name)and may scan the entire ecosystem to resolve at most 25 names. Split the small input list into namespace/name columns and join on the indexed expressions instead.
03007ce to
df65abe
Compare
| force: input.force, | ||
| error: errorMessage, | ||
| }) | ||
| throw ApplicationFailure.nonRetryable(errorMessage, 'BLAST_RADIUS_STAGE_FAILED') |
There was a problem hiding this comment.
Done analyses marked failed
High Severity
If a late pipeline stage error runs after finalizeAnalysis has set status to done, the workflow catch still calls failAnalysis, which unconditionally overwrites status to failed. Poll only loads verdicts when status is done, so a finished run can appear failed with no summary or results even though verdict rows remain in the database.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit df65abe. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 7 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (12)
services/apps/packages_worker/src/blast-radius/stages/intel.ts:70
- When a package was explicitly requested but is not present in the advisory, this expression silently falls back to the advisory's first npm entry. The analysis then examines a different package while the poll response still reports the requested package. Only use the first entry when no package was requested; otherwise fail on a mismatch.
const entry =
(requestedPackage && npmEntries.find((e) => e.package.name === requestedPackage)) ||
npmEntries[0]
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:190
- All abbreviated-registry errors, including rate limits and transient network failures, are treated as a clean non-match. During an npm outage this can produce an empty candidate list and a successful zero-impact report. Ignore only genuine
NOT_FOUNDresponses and propagate other error kinds so Temporal can retry the stage.
const packument = await fetchAbbreviatedPackument(name)
if (isFetchError(packument)) return
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:283
- Transient/rate-limit failures from the full packument fetch are also silently interpreted as "not a dependent." This can remove high-ranked dependents from the result while the stage still succeeds. Skip only
NOT_FOUND; let other upstream failures fail and retry the activity.
const packument = await fetchPackument(name)
if (isFetchError(packument)) continue
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:322
- The downstream stage persists only
excludedByRange, and the GET endpoint derives its public exclusion count from those rows. Slicing this list to 200 therefore makesdependentsExcludedUpfrontand the total under-report whenever more than 200 candidates fail the range check, even though the uncapped count is computed here. Persist the uncapped count as analysis metadata (or persist all exclusions) and use it in the response.
excludedByRange: excludedByRange.slice(0, 200),
excludedByRangeCount: excludedByRange.length,
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:244
- The npm downloads API range is inclusive at both ends. Subtracting 30 days while also including today requests 31 calendar days, so ranking and the
downloadsLast30Daysresponse field are off by one day.
const rangeEnd = new Date()
const rangeStart = new Date(rangeEnd)
rangeStart.setUTCDate(rangeStart.getUTCDate() - 30)
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:253
- After the pre-scan, the download batches and sequential full-packument walk never call
onProgress. Each request can wait 30 seconds, so seven slow batches/fetches exceed the activity's 3-minute heartbeat timeout and trigger an overlapping retry even though this attempt is still working. Heartbeat after each bulk/scoped/full-packument request.
for (let i = 0; i < unscopedNames.length; i += 128) {
const batch = unscopedNames.slice(i, i + 128)
const result = await fetchBulkPointRange(batch, isoDate(rangeStart), isoDate(rangeEnd))
services/libs/data-access-layer/src/packages/osv.ts:139
- This join reconstructs a full name from package columns, so it cannot use the existing
(ecosystem, COALESCE(namespace, ''), name)index (V1779710880__initial_schema.sql:82). For npm's large package population, each analysis can scan all npm rows just to resolve at most 25 IDs. Parse the input into namespace/name pairs and join on the indexed columns instead.
backend/src/api/public/v1/packages/submitBlastRadiusJob.ts:42 - The
trystarts aftergetPackagesTemporalClient(). If client initialization rejects (for example, Temporal is unreachable), the analysis row created above remainspendingforever andfailAnalysisis never called—the exact failure mode this catch is intended to prevent. Move client acquisition inside thetry.
try {
await packagesTemporal.workflow.start('analyzeBlastRadius', {
services/libs/data-access-layer/src/packages/blastRadius.ts:78
- Both columns are BIGINT, but this connection only registers int4 as a numeric parser (
services/libs/database/src/connection.ts:84-85), so pg-promise returns these IDs as strings. Typing them asnumberrisks precision loss and conflicts with the string ID handling introduced inosv.ts:125-143. Update the row types and the relatedpackageIdparameter/lookup return type tostring | null.
advisory_id: number | null
package_id: number | null
backend/src/api/public/v1/akrites-external/openapi.yaml:473
- This description is internally contradictory: the implementation adds rows explicitly excluded because their declared ranges do not include a vulnerable version, so the sum is not a count of dependents that "fell within" the vulnerable range. Describe it as the evaluated range-walk population (including range exclusions), or change the field computation/name to represent in-range dependents.
totalDependentsInRange:
type: integer
description: Sum of dependentsAnalyzed and dependentsExcludedUpfront; the count of dependents that fell within the vulnerable version range (before top-N cutoff was applied).
services/apps/packages_worker/src/blast-radius/clients/npmTarball.ts:87
- Archive-created symlinks are followed both by this common-prefix
statSyncand later bystatSyncincopyTreeGuarded. A tarball can pointpackage/leak(or the sole top-level entry) at a host path and copy host files into the agent tree despite the destination check. UselstatSyncat both sites and reject symlinks/special files, copying regular files only.
const items = fs.readdirSync(tempDir)
const commonPrefix =
items.length === 1 && fs.statSync(path.join(tempDir, items[0])).isDirectory()
? items[0]
: null
services/apps/packages_worker/src/blast-radius/agent/runner.ts:68
- These tools are auto-allowed without a path boundary, while the analyzed npm source is untrusted and the subprocess inherits the worker environment. A prompt-injected package can induce reads outside
cwd(including mounted credentials or/proc/.../environ) and send them to the model. Enforce cwd-only tool authorization or an isolated filesystem/user, and pass only an allowlisted environment.
tools: ['Read', 'Grep', 'Glob'],
disallowedTools: ['Bash', 'Write', 'Edit', 'NotebookEdit', 'WebFetch', 'WebSearch', 'Task'],
| if (dependents.length === 0) return | ||
| for (const dep of dependents) { | ||
| await qx.result( |
|
|
||
| # security-contacts-worker | ||
| SECURITY_CONTACTS_USER_AGENT="lfx-security-contacts-worker" | ||
| CROWD_PACKAGES_TEMPORAL_SERVER_URL=temporal:7233 No newline at end of file |
50a875b to
6bf837f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (8)
services/apps/packages_worker/src/blast-radius/agent/runner.ts:73
cwdis not a filesystem sandbox, and pre-allowingRead/Grep/Globlets the agent access paths outside the extracted package. Because package source is untrusted and the child also receives the worker environment, a prompt-injected package can read files such as/proc/self/environand disclose service credentials to the model. Remove the blanket allow and enforce a canonicalizedcwdpath boundary (or run the agent in an isolated container with only the package mounted) before processing third-party source.
allowedTools: ['Read', 'Grep', 'Glob'],
services/apps/packages_worker/src/blast-radius/stages/intel.ts:70
- When a package was explicitly requested but is not one of the advisory's npm entries, this fallback silently analyzes
npmEntries[0]. The stored/APIpackage_namestill reports the requested package, so clients receive blast-radius results for a different package. Only fall back when no package was requested; otherwise fail the analysis.
const entry =
(requestedPackage && npmEntries.find((e) => e.package.name === requestedPackage)) ||
npmEntries[0]
backend/src/api/public/v1/packages/submitBlastRadiusJob.ts:39
getPackagesTemporalClient()can itself throw or reject when configuration is missing or the Temporal connection fails, but it runs outside thistry. In that case the pending row created above is never marked failed, which is the stuck state this handler is intended to prevent. Acquire the client inside the sametry.
const packagesTemporal = await getPackagesTemporalClient()
backend/.env.dist.composed:44
- The composed backend still cannot construct
PACKAGES_TEMPORAL_CONFIG:backend/src/conf/index.ts:91-95only enables it whenpackagesTemporal.namespaceexists, but this file adds only the server URL. AddCROWD_PACKAGES_TEMPORAL_NAMESPACE(the local template usesdefault) or every composed submission will fail before starting the workflow.
CROWD_PACKAGES_TEMPORAL_SERVER_URL=temporal:7233
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:238
- Progress callbacks are only emitted by the phase-1 pre-scan. The download-count loops and especially the sequential full-packument walk below can each run longer than the activity's 3-minute heartbeat timeout (each request has a 30-second timeout), causing Temporal to time out and retry healthy work. Heartbeat on a time-based cadence throughout every network loop, not only every 200 phase-1 items.
// Phase 1: concurrent, lightweight pre-scan — filters the (potentially ~17k) high-impact
// list down to the names that actually declare a dependency on the target, before doing
// any of the more expensive per-candidate work below.
const candidateNames = await candidateNamesFromScan(toScan, targets, onProgress)
backend/src/api/public/v1/packages/blastRadiusAnalysis.ts:114
- This adds packages explicitly marked
excluded_by_rangetototalDependentsInRange, so the public field reports out-of-range packages as in-range. Either make this value represent only in-range analyzed dependents, or rename the public field/description to a total-evaluated metric; the current result contradicts its API meaning.
totalDependentsInRange: dependentsAnalyzed + dependentsExcludedUpfront,
services/apps/packages_worker/src/blast-radius/stages/intel.ts:143
- An Agent SDK error result can still carry nonzero
costUsd, but this branch throws it away andfailStageRunstores no cost. If Temporal retries the activity and the next attempt succeeds, the final report includes only the successful call and underreports actual spend. Accumulate/persist failed-attempt cost as the reachability stage already does.
if (agentResult.isError || !agentResult.structuredOutput) {
throw new Error(`Agent failed: ${agentResult.errorMessage}`)
}
backend/src/api/public/v1/akrites-external/openapi.yaml:485
- The implementation calculates this percentage over conclusive verdicts and returns null when every analyzed result is
unclear, even whendependentsAnalyzedis nonzero. Document that denominator/null condition so API consumers do not infer different behavior from the schema.
description: Rounded to 1 decimal. Null when dependentsAnalyzed is 0, to avoid a misleading 0%.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (8)
backend/.env.dist.composed:44
- The composed environment still lacks
CROWD_PACKAGES_TEMPORAL_NAMESPACE.PACKAGES_TEMPORAL_CONFIGis only initialized when that namespace exists (backend/src/conf/index.ts:91-95), so the composed API will treat packages Temporal as unconfigured and every submission will fail before connecting. Define the namespace alongside the new server URL.
CROWD_PACKAGES_TEMPORAL_SERVER_URL=temporal:7233
backend/src/api/public/v1/packages/submitBlastRadiusJob.ts:39
getPackagesTemporalClient()can itself reject when configuration is missing or the connection fails, but it runs before thistry. In that case the row created above remainspendingforever, which is the exact state the catch is intended to prevent. Move client acquisition inside the guarded block.
const packagesTemporal = await getPackagesTemporalClient()
services/apps/packages_worker/src/blast-radius/stages/intel.ts:70
- When a caller narrows a multi-package advisory to a package that is not present in OSV, this expression silently falls back to the first entry. The analysis is then persisted and returned under the requested package name even though a different package was analyzed. Only use the first entry for advisory-wide requests; reject a requested package that does not match.
const entry =
(requestedPackage && npmEntries.find((e) => e.package.name === requestedPackage)) ||
npmEntries[0]
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:322
- The API derives
dependentsExcludedUpfrontby counting persisted excluded rows, but this truncates those rows to 200 even thoughexcludedByRangeCountalready records the real count. Analyses with more than 200 exclusions therefore return an undercounted summary. Persist the uncapped count as analysis metadata (or otherwise use it in the read path) while retaining the row cap.
excludedByRange: excludedByRange.slice(0, 200),
excludedByRangeCount: excludedByRange.length,
services/libs/data-access-layer/src/packages/osv.ts:139
- This join applies concatenation to every
packagesrow, so PostgreSQL cannot use the existing(ecosystem, COALESCE(namespace, ''), name)index for the name lookup; it can only narrow by ecosystem and scan all npm packages. Parse each input into namespace/name on theunnestside and join on the indexed columns instead.
services/libs/data-access-layer/src/packages/blastRadius.ts:332 - This performs one database round-trip per dependent, sequentially—up to 225 INSERT/UPSERT calls for a single analysis. The DAL already provides
prepareBulkInsert; use a batched upsert (chunked if necessary) so persistence is one or a small bounded number of queries.
for (const dep of dependents) {
await qx.result(
backend/src/api/public/v1/akrites-external/openapi.yaml:485
- This description does not match the implementation: the percentage is computed over conclusive verdicts only and becomes null when every analyzed result is
unclear, even whendependentsAnalyzedis nonzero. Document the actual denominator so API consumers interpret the metric correctly.
affectedPercentage:
type: number
nullable: true
description: Rounded to 1 decimal. Null when dependentsAnalyzed is 0, to avoid a misleading 0%.
backend/src/api/public/v1/akrites-external/openapi.yaml:1099
- This GET route uses the shared 60/min
rateLimiter, not the independent blast-radius limiter used by POST. The documented 429 behavior therefore incorrectly says polling is independently rate-limited. Either give polling its own limiter or update the contract (including the Blast Radius tag description) to describe the shared limit.
'429':
description: Too many requests — rate-limited independently of the other endpoints.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (10)
backend/src/api/public/v1/packages/submitBlastRadiusJob.ts:39
getPackagesTemporalClient()can itself reject while connecting (or throw when configuration is missing), but it runs before thistry. In that failure mode the row created above remainspendingforever, despite the catch being intended to handle Temporal unavailability. Move client acquisition inside the guarded block.
const packagesTemporal = await getPackagesTemporalClient()
services/apps/packages_worker/src/blast-radius/stages/intel.ts:70
- When a package was explicitly requested but is absent from the advisory, this expression silently falls back to the advisory's first npm package. The persisted/API
package_namestill contains the requested package, so the completed results are attributed to the wrong package. Only use the first entry for advisory-wide requests; reject an unmatched explicit package.
const entry =
(requestedPackage && npmEntries.find((e) => e.package.name === requestedPackage)) ||
npmEntries[0]
services/apps/packages_worker/src/blast-radius/stages/intel.ts:143
- An agent error result can still contain billed
costUsd, but this branch throws before recording it andfailStageRunalways leaves the stage cost at zero. Failed Intel attempts—and activity retries after them—are therefore omitted from the final analysis cost. Track the returned cost and persist it on failed stage runs as Reachability does for failed attempts.
if (agentResult.isError || !agentResult.structuredOutput) {
throw new Error(`Agent failed: ${agentResult.errorMessage}`)
}
services/libs/data-access-layer/src/packages/osv.ts:139
- This join compares a computed concatenation of package columns, so PostgreSQL cannot directly use the existing unique index on
(ecosystem, COALESCE(namespace, ''), name)(V1779710880__initial_schema.sql:82). It can scan every npm package for each analysis. Split each input into namespace/name expressions so the indexed package columns remain equality operands.
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:186 - With 32 workers and a 30-second request timeout, the 200th request may not start until roughly 180 seconds during a registry slowdown—the activity's entire heartbeat timeout. Temporal can then retry a still-running scan. Heartbeat once initially and about once per concurrency wave instead of once per 200 requests.
await mapWithConcurrency(names, SCAN_CONCURRENCY, async (name) => {
processed++
if (onProgress && processed % 200 === 0) {
onProgress()
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:190
- All fetch failures—including
RATE_LIMITandTRANSIENT—are treated as proof that the package is not a dependent. A registry outage can therefore produce an empty/partial candidate set and a successful zero-impact report. Distinguish terminal per-package failures from transient/rate-limit failures, and retry or fail the stage when scan completeness is compromised.
const packument = await fetchAbbreviatedPackument(name)
if (isFetchError(packument)) return
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:283
- A transient or rate-limited full-packument fetch silently removes this ranked candidate from consideration, so the resulting top-N can omit the highest-impact dependents while the analysis still finishes successfully. Retry/fail on transient errors and reserve
continuefor terminal package-specific failures.
const packument = await fetchPackument(name)
if (isFetchError(packument)) continue
services/apps/packages_worker/src/blast-radius/dependentsScan.ts:322
- Only the first 200 range exclusions are returned and persisted, while the public poll endpoint derives
dependentsExcludedUpfrontby counting those rows. Analyses with more than 200 exclusions therefore publish a silently capped summary even thoughexcludedByRangeCountalready has the real count. Persist the aggregate count separately (or remove the cap) and use it in the read path.
excludedByRange: excludedByRange.slice(0, 200),
excludedByRangeCount: excludedByRange.length,
services/apps/packages_worker/src/blast-radius/clients/npmTarball.ts:50
- The extraction limits can be bypassed with links: tar hardlink/symlink entries commonly report size 0, then
copyTreeGuardedfollows them viastatSync/copyFileSyncand materializes each copy. An archive containing many links to one large file can therefore expand far beyondMAX_EXTRACTED_BYTESand exhaust worker disk. Reject link entries (or account for bytes while copying).
filter: (_entryPath, entry) => {
extractedFiles++
extractedBytes += entry.size ?? 0
backend/src/api/public/v1/akrites-external/openapi.yaml:473
- This contract does not match the returned value: the implementation adds analyzed rows to rows explicitly marked
excluded_by_range, and the scan stops after finding the top 25 analyzed packages. The number therefore includes packages that did not fall within the vulnerable range and excludes candidates not reached before the cutoff. Rename/redefine the field or change the computation before publishing this new API.
totalDependentsInRange:
type: integer
description: Sum of dependentsAnalyzed and dependentsExcludedUpfront; the count of dependents that fell within the vulnerable version range (before top-N cutoff was applied).
| for (const name of ranked) { | ||
| if (analyzed.length >= topN) break | ||
|
|
||
| const packument = await fetchPackument(name) |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 00de0ed. Configure here.
| dependentsAnalyzed, | ||
| dependentsAffected: affected.length, | ||
| affectedPercentage: | ||
| conclusive.length > 0 ? Math.round((affected.length / conclusive.length) * 1000) / 10 : null, |
There was a problem hiding this comment.
Summary percentage contract mismatch
Medium Severity
OpenAPI states affectedPercentage is null only when dependentsAnalyzed is zero, but toSummary returns null when every verdict is unclear, even if dependents were analyzed. Clients can see a non-zero dependentsAnalyzed with a null percentage contrary to the published contract.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 00de0ed. Configure here.
5f124f3 to
00de0ed
Compare


Summary
Completes the blast-radius analysis pipeline implementation (CM-1328) by porting the full 4-stage methodology from the Python PoC into the Temporal worker. Includes database schema migrations (from prior PR on this branch), complete data-access-layer for querying/persisting analysis state, and all 4 pipeline stages (intel, dependents, reachability, report) with Agent SDK integration for vulnerability intelligence and reachability analysis. Replaces the PoC's JSON-file-based run artifacts with database-backed resumable state, and integrates Opus/Sonnet agents for the knowledge-intensive stages (Stage 1 intel and Stage 3 per-dependent reachability).
Changes
Database (migrations, already in this branch)
blast_radius_analyses,blast_radius_symbol_specs,blast_radius_dependents,blast_radius_verdicts,blast_radius_stage_runs.DAL —
services/libs/data-access-layer/src/packages/blastRadius.tsSemver range helpers —
services/apps/packages_worker/src/blast-radius/semverRange.tsversionsInRanges,rangeIncludesAny(with unparseable-range detection),highestVersion.semverpackage (already a dependency).Clients —
services/apps/packages_worker/src/blast-radius/clients/.patchfiles from GitHub commit/PR URLs.Readable.fromWeb→.pipe()) intotar.extract()'s writable stream, then copy the extracted tree intodestDirwith a path-traversal guard, stripping the package's common top-level directory.npm registry reuse —
npmManifest.tsPackumentVersiontype (used by the ingest pipeline) only declares a few fields; blast-radius also needsdependencies/peerDependencies/optionalDependencies/dist.tarball. Rather than widen the shared type,npmManifest.tsdeclares a localNpmVersionManifest extends PackumentVersionplus anasNpmVersionManifest()cast helper, used at every access site.fetchPackument/fetchBulkPointRange/isFetchErrorclients insrc/npm/instead of ad-hocfetch()calls.Dependents scan —
dependentsScan.tsnpm-high-impactpackage list, extract candidate names, rank by downloads (last calendar month, computed at runtime — not a hardcoded date range), filter by declared dependency + range inclusion.{source, candidatesConsidered, analyzed[], excludedByRange[]}; matches PoC's Stage 2 logic.Agent integration —
services/apps/packages_worker/src/blast-radius/agent/@anthropic-ai/claude-agent-sdk'squery()with:BLAST_RADIUS_ANTHROPIC_API_KEYat runtime; if present, passes asenvoption; if absent, subprocess inheritsprocess.envfor CLI auth.tools: ['Read','Grep','Glob']+disallowedToolsbelt-and-suspenders.outputFormat: {type:'json_schema', schema}.permissionMode: 'bypassPermissions'+allowDangerouslySkipPermissions: true.AbortController.Stages —
services/apps/packages_worker/src/blast-radius/stages/scanDependents, persist analyzed + excluded-by-range rows.fs.mkdtemp, 3 retries with 15×attemptNumber sec backoff, claude-sonnet-5 agent. Error verdicts on persistent failure. AtoPromptSymbolSpec()converter maps the DAL's raw DB row onto the prompt-facingSymbolSpectype instead of casting.Activities + workflow —
services/apps/packages_worker/src/blast-radius/blastRadiusStart/blastRadiusFailown all analysis-lifecycle DB writes (create/mark-running/fail), plus the 4 stage-wrapping activities (Intel, Dependents, Reachability, Report).analyzeBlastRadiuswith real 4-stage orchestration viaproxyActivities. The workflow body does no DB/DAL access directly (Temporal determinism) — all lifecycle and stage work goes through proxied activities, looped over[stageName, runFn]tuples with per-stage failure handling.services/apps/packages_worker/src/activities.tsfor worker discovery.Design decisions (non-obvious)
envoption replaces rather than mergesprocess.env, so we build a merged env with the key if present, or omitenventirely to inherit.stage_runs.model.fs.mkdtempdir, cleaned up infinallyblock post-verdict. Avoids shared-state accumulation.stage_run.statusnot file presence.Status
pnpm tsc-check/pnpm lint) in bothpackages_workeranddata-access-layer, noanywarnings.packages_worker(18 in blast-radius: 16 semverRange + 2 ecosystemSupport), no regressions elsewhere.Limitations / future work
Type of change
JIRA ticket
CM-1328